Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

matchers: support null value in expression matcher #6320

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohammed90
Copy link
Member

The plain header match (non-CEL) supports checking for the absence of a header. The corresponding CEL matcher couldn't achieve the same because it expects a string a slice in the value of header->value map. In the plain header matcher, the absence of a value is represented by a nil in-place of zero-slice as the value of the header key in the map. The nil only equals nil when iterating through the map. We needed an analogous value, i.e. null.

My attempt here is to add support for null. I'm stumbling in the dark as I'm not familiar with the cel-go module, but something works, so am I close?

Here are the checks I've run:

~ $ # Before: using `@without `header({"X-Custom-Header": ""})``
~ $ curl -k https://localhost
with
~ $ curl -k -H "X-Custom-Header:" https://localhost
with
~ $ curl -k -H "X-Custom-Header: d" https://localhost
with

~ $ # Before: using `@without `header({"X-Custom-Header": [] })``
2024-05-17 1:20:57  ~ $ curl -k https://localhost
with
~ $ curl -k -H "X-Custom-Header:" https://localhost
with
~ $ curl -k -H "X-Custom-Header: val" https://localhost
without

~ $ # After: using ``
~ $ curl -k https://localhost
without
~ $ curl -k -H "X-Custom-Header:" https://localhost
without
~ $ curl -k -H "X-Custom-Header: val" https://localhost
with

So, works?

@mohammed90 mohammed90 added feature ⚙️ New feature or request in progress 🏃‍♂️ Being actively worked on discussion 💬 The right solution needs to be found needs tests 💯 Requires automated tests labels May 16, 2024
@mohammed90
Copy link
Member Author

mohammed90 commented May 16, 2024

@francislavoie
Copy link
Member

I think this is making all strings nullable? I think that's probably not what we want 🤔 We might need to make a specific function to handle null but only for handle (or a different set of handlers). It's crashing with a SIGSEGV which is kinda wild but I guess not totally surprising with null lol

@TristonianJones FYI in case you have any suggestions. The gist of it is we're trying to allow header({"X-Customer-Token": null}) I think (i.e. null values in "JSON-like" config, but probably only for this one matcher/function)

@@ -704,7 +708,7 @@ var (
placeholderRegexp = regexp.MustCompile(`{([a-zA-Z][\w.-]+)}`)
placeholderExpansion = `caddyPlaceholder(request, "${1}")`

CELTypeJSON = cel.MapType(cel.StringType, cel.DynType)
CELTypeJSON = cel.MapType(cel.StringType, cel.AnyType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't modify this line. The DynType is really more accurate here.

@TristonianJones
Copy link
Contributor

I think this is making all strings nullable? I think that's probably not what we want 🤔 We might need to make a specific function to handle null but only for handle (or a different set of handlers). It's crashing with a SIGSEGV which is kinda wild but I guess not totally surprising with null lol

@TristonianJones FYI in case you have any suggestions. The gist of it is we're trying to allow header({"X-Customer-Token": null}) I think (i.e. null values in "JSON-like" config, but probably only for this one matcher/function)

Hi @francislavoie, I think part of this issue is that the internal types are map[string]string and map[string][]string even though the CEL type is a map(string,dyn). If you want to use null to represent absent, then you can absolutely do this; however, doing so would require handling null everywhere it could appear, at least as far as the macro is concerned. It's usually a bad idea to relax the constraints conditionally, in one case, but not all cases. Can you omit the key and use 'key-name' in map instead as the expression for testing existence?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found feature ⚙️ New feature or request in progress 🏃‍♂️ Being actively worked on needs tests 💯 Requires automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants